Skip to content

[toolchain] Harden installation failure handling and RapidJSON CMake support#7583

Open
QuantumMisaka wants to merge 12 commits into
deepmodeling:developfrom
QuantumMisaka:toolchain-issues
Open

[toolchain] Harden installation failure handling and RapidJSON CMake support#7583
QuantumMisaka wants to merge 12 commits into
deepmodeling:developfrom
QuantumMisaka:toolchain-issues

Conversation

@QuantumMisaka

@QuantumMisaka QuantumMisaka commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Linked Issue

Fix #7565 #7566 #7569

Summary

This PR hardens the ABACUS toolchain installation flow for issues found during toolchain/install review.

Changes include:

  • Preserve non-zero installer failures through the top-level toolchain wrappers
  • Propagate argument parsing failures instead of continuing silently
  • Install RapidJSON with a CMake package config so ABACUS can consume an installed RapidJSON dependency reliably
  • Add focused shell/CMake regression tests for the fixed behavior

Root Cause

The wrapper scripts could mask failures from lower-level installers or argument validation paths, making a failed install appear successful.

RapidJSON was installed as headers only, which left CMake package discovery fragile for builds that expect an installed package layout.

Validation

Validated on the SAI CPU-MISC/rush-cpu environment from commit 583c5dc66.

Checks run:

  • toolchain/tests/test_wrapper_failure_propagation.sh
  • toolchain/tests/test_installer_argument_failures.sh
  • toolchain/tests/test_rapidjson_cmake.sh
  • Full toolchain/toolchain_gnu.sh -j 2 installation
  • ./build_abacus_gnu.sh -j 2 with RapidJSON enabled
  • Real ABACUS run of examples/02_scf/01_pw_Si2

The final real calculation job was Slurm 619964, completed with ExitCode=0:0, five SCF iterations (DS1-DS5), and TOTAL Time : 2 in ABACUS output.

Note: the full verification environment hit intermittent GitHub/proxy TLS EOFs while fetching some release tarballs. After supplying checksum-matching source archives, the same installation
continued and passed. These network failures also confirmed that wrapper-level failures now propagate as non-zero exits.

Growl1234
Growl1234 reviewed Jul 3, 2026
View reviewed changes
Comment thread cmake/AbacusRapidJSON.cmake Outdated
@Growl1234

Growl1234 commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown

RapidJSON was installed as headers only, which left CMake package discovery fragile for builds that expect an installed package layout.

If the system-installed package has a incomplete CMake configuration, for example, RapidJSON installed from dnf pulls in an invalid one, we should consider blocking system detection or report error when wanted target is not present, rather than adapting to the already fragile case.

Growl1234
Growl1234 reviewed Jul 3, 2026
View reviewed changes
Comment thread CMakeLists.txt
@mohanchen mohanchen added the Compile & CICD & Docs & Dependencies Issues related to compiling ABACUS label Jul 3, 2026
Growl1234
Growl1234 reviewed Jul 4, 2026
View reviewed changes

@Growl1234 Growl1234 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I don't think it's worth bringing in too many helpers just for testing small case; the maintenance burden of toolchain is already high, and fully assigning it to an AI agent should have not been really a good idea...

At least, keeping only non-overlapping behavioral checks would be clearer and less brittle.

Comment thread toolchain/tests/test_rapidjson_cmake.sh Outdated
Comment thread toolchain/tests/test_rapidjson_cmake.sh Outdated
@Growl1234 Growl1234 mentioned this pull request Jul 4, 2026
Merged
4 tasks
Growl1234
Growl1234 reviewed Jul 4, 2026
View reviewed changes
Comment thread CMakeLists.txt Outdated
Growl1234
Growl1234 approved these changes Jul 5, 2026
View reviewed changes

@Growl1234 Growl1234 left a comment •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, just a minor concern :)

Consider using "Fix #7565, fix #7566, fix #7569" in the description so that all issues are actually linked.

Comment thread toolchain/tests/test_rapidjson_cmake.sh Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mohanchen mohanchen mohanchen approved these changes
+1 more reviewer
@Growl1234 Growl1234 Growl1234 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Compile & CICD & Docs & Dependencies Issues related to compiling ABACUS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Code scan] Preserve installer failures through toolchain wrapper logging pipelines

3 participants

@QuantumMisaka @Growl1234 @mohanchen